[fix](be) Report stream load size limits in MiB - #66224
Open
0AyanamiRei wants to merge 3 commits into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: JSON Stream Load body-size errors displayed both byte counts without units next to a configuration named streaming_load_json_max_mb, making the configured limit misleading. Keep the existing threshold and byte comparison unchanged, label both exact byte values, and show the snapshotted configured MB value. Add a BE unit test using the reported request size.
### Release note
Clarify units in the JSON Stream Load body-size limit error message.
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=StreamLoadTest.* -j48 (2 tests passed)
- Behavior changed: Yes. The JSON Stream Load size-limit error now identifies byte values and the configured MB value.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Stream Load size-limit errors mixed byte counts with configuration names expressed in MB, which made both JSON and CSV failures misleading. Report the actual request size and configured limit consistently in MiB for JSON Stream Load, CSV Stream Load, and HTTP Stream without changing any thresholds or rejection behavior. Add BE unit coverage for all three paths.
### Release note
Report Stream Load body-size limit errors consistently in MiB.
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=StreamLoadTest.* -j48 (4 tests passed)
- Behavior changed: Yes. JSON and CSV Stream Load size-limit errors now display both values in MiB.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28941 ms |
Contributor
TPC-DS: Total hot run time: 176946 ms |
Contributor
ClickBench: Total hot run time: 25.16 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The JSON and CSV Stream Load size-limit errors did not use the requested wording for their BE configuration references. Update both messages to name BE's conf directly while preserving the existing limits and MiB conversion, and adjust the exact unit-test expectations.
### Release note
Improve JSON and CSV Stream Load size-limit error wording.
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter='StreamLoadTest.*' -j48
- Behavior changed: Yes (error message wording only)
- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review conclusion: request changes.
The change is small and focused, and the byte thresholds and rejection behavior are otherwise preserved. I found one user-visible correctness problem in the new diagnostics: requests just over a limit can be displayed as exactly equal to it because the body size is rounded to two fractional MiB digits. The inline comment covers the identical JSON Stream Load, CSV Stream Load, and HTTP Stream expressions.
Critical checkpoint conclusions:
- Goal and proof: All three errors are converted to MiB and the new unit tests cover a representative 108.15 MiB rejection, but the fixed precision fails the exact over-limit boundary and there is no limit-plus-one test.
- Scope and parallel paths: The implementation is localized. JSON line mode, CSV handling, HTTP Stream, chunked/no-Content-Length behavior, and the two mutable limit configs were reviewed; no distinct parallel-path regression was found.
- Concurrency and lifecycle: The new values are request-local snapshots. The tests restore the mutable globals, use supported BE-UT private access, and return before their nullptr ExecEnv values are dereferenced; no new lock, lifecycle, or ownership issue was found.
- Configuration and compatibility: No config is added. Supported-value thresholds and per-request dynamic-config behavior are unchanged. The error text changes intentionally; error codes, protocol/storage formats, and rolling-upgrade behavior are unaffected.
- Data correctness, transactions, and FE/BE propagation: The change only formats a pre-transaction rejection and adds no transmitted or persisted state, so these invariants are unaffected.
- Performance and observability: Conversion occurs only on the error path and adds no material cost. Existing error status and warning behavior remain sufficient.
- Tests: The author reports
./run-be-ut.sh --run --filter=StreamLoadTest.* -j48passing four tests. Per the review-runner instructions, this review was static and did not run builds or tests. Boundary coverage is still required for the accepted finding.
There was no additional user-provided review focus.
| "json body size {} exceed BE's conf `streaming_load_json_max_mb` {}. increase " | ||
| "it if you are sure this load is reasonable", | ||
| ctx->body_bytes, json_max_body_bytes); | ||
| "json body size {:.2f} MiB exceeds the limit of {} MiB set by BE's conf " |
Contributor
There was a problem hiding this comment.
The byte comparison is exact, but {:.2f} can round away the excess that triggered it. With a 100 MiB limit, Content-Length: 104857601 is rejected while this renders 100.00 MiB exceeds the limit of 100 MiB; the CSV branch and HttpStreamAction have the same issue. Please retain the exact byte count (for example alongside the friendly MiB value), or use a rendering policy that cannot display an over-limit value as equal, and add limit-plus-one coverage for all three paths.
Contributor
TPC-H: Total hot run time: 29650 ms |
Contributor
TPC-DS: Total hot run time: 177269 ms |
Contributor
ClickBench: Total hot run time: 24.76 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Stream Load body-size limit errors display raw byte counts next to BE configuration items whose names and values are expressed in MB, making the message misleading. Report both the actual body size and configured limit in MiB for JSON Stream Load, CSV Stream Load, and HTTP Stream. The limit calculation and rejection behavior are unchanged.
Release note
Report Stream Load body-size limit errors consistently in MiB.
Check List (For Author)
Test
./run-be-ut.sh --run --filter=StreamLoadTest.* -j48 (4 tests passed)
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)